I think true might be a more natural default value for this?

Changed to true. The false default value was to make the tests in "ranking across signals" and "final results end-to-end" of "ChannelLinkAutocompleteView" group a little less verbose.🙂

channel: Add isRecentlyActive field

Since we're already not matching the order in the API doc (see e.g. #1894 (comment) ), I'd put this next to the related-looking field streamWeeklyTraffic, perhaps just above it without an empty line in between.

Similarly elsewhere in this commit.

We try to reserve assert for invariants that are our own responsibility, i.e. those that won't be broken except for some broken logic in the client. Here, the invariant exists, but it's one that can be broken by something out of our control, in particular a badly-behaving server.

Also, asserts don't run in production, so this won't work as "crunchy shell" validation. It makes sense to want such validation, so the "soft center" of the app can rely on this invariant. But let's do it in ChannelDeleteEvent.fromJson; for an example to follow, see DeleteMessageEvent.fromJson.

nit: we'd normally say TODO(server-10) make required or just TODO(server-10).

Oh actually, we can make this nicer by encapsulating this conditional in the API-binding layer—ChannelDeleteEvent can have just final List<int> streamIds (maybe channelIds, as the more modern name?), and it can read its value depending on what the JSON looks like.

Something like this (untested)?:

/// A [ChannelEvent] with op `delete`: https://zulip.com/api/get-events#stream-delete
@JsonSerializable(fieldRename: FieldRename.snake)
class ChannelDeleteEvent extends ChannelEvent {
  @override
  @JsonKey(includeToJson: true)
  String get op => 'delete';

  @JsonKey(readValue: _readChannelIds)
  final List<int> channelIds;

  // TODO(server-10) simplify away; rely on stream_ids
  static List<int> _readChannelIds(Map<dynamic, dynamic> json, String key) {
    final channelIds = json['stream_ids'] as List<int>?;
    if (channelIds != null) return channelIds;

    final channels = json['streams'] as List<dynamic>;
    return channels
      .map((c) => (c as Map<String, dynamic>)['stream_id'] as int)
      .toList();
  }

  ChannelDeleteEvent({
    required super.id,
    required this.channelIds,
  });

  factory ChannelDeleteEvent.fromJson(Map<String, dynamic> json) =>
    _$ChannelDeleteEventFromJson(json);

  @override
  Map<String, dynamic> toJson() => _$ChannelDeleteEventToJson(this);
}

(In that code, the crunchy-shell validation is done by the final channels = json['streams'] as List<dynamic>; line, which will throw if both .stream_ids and .streams are absent in the json.)

Changed to the new version. One thing that this does is that in toJson, there will be an entry with key channel_ids; not exactly what the server gives us (stream_ids or streams). Should we edit the toJson method to match the server data, or is it not that important since we don't use it to send it back to the server?

Ahh, so there was a test failing in test/model/store_test.dart and the fix was to include streams in toJson.

Hmm, streams is deprecated and may be removed, so I think we should treat it as valid if streams is absent and stream_ids is present. I'd want our tests to tolerate that form without failing.

I took a look and found a bug in _readChannelIds in this revision 🙂:

diff --git lib/api/model/events.dart lib/api/model/events.dart
index 6a0d9ffa4..4d9c5121c 100644
--- lib/api/model/events.dart
+++ lib/api/model/events.dart
@@ -622,7 +622,7 @@ class ChannelDeleteEvent extends ChannelEvent {
   // TODO(server-10) simplify away; rely on stream_ids
   static List<int> _readChannelIds(Map<dynamic, dynamic> json, String key) {
     final channelIds = json['stream_ids'] as List<dynamic>?;
-    if (channelIds != null) channelIds.map((id) => id as int).toList();
+    if (channelIds != null) return channelIds.map((id) => id as int).toList();
 
     final channels = json['streams'] as List<dynamic>;
     return channels

Neat! Unfortunately we're not ready to show the rendered channel description; some of our content widgets will break if they appear outside the context of a message, because they use InheritedMessage.of(context), and we need to address that systematically, which is #488. See related issues:

• Show channel description in channel action sheet #1896
• content: Support Zulip content outside messages (even outside per-account contexts) #488

For now let's do as I did in #1877:

• Not try to show the channel description here
• File an issue for it and leave a TODO

Filed #1945. Also, looking at https://github.com/zulip/zulip-flutter/blob/main/lib/widgets/content.dart, it seems like InheritedMessage.of(context) is used in two places, MessageImagePreview and MessageInlineVideo, and by manually testing, it seems like the server is not sending the related HTML for rendering these widgets when there is an image or video in the channel description. So I think it will be safe to show the channel description now. But as it’s possible that InheritedMessage.of(context) will be used in other widgets, it's good to wait for #488 as you mentioned.

by manually testing, it seems like the server is not sending the related HTML for rendering these widgets when there is an image or video in the channel description. So I think it will be safe to show the channel description now.

For this sort of detail I'd want to rely on API guarantees rather than manual testing on a current server. It's at least plausible that we could run into different behavior with some old server we still support (like 7), or even on any server, including a modern one like CZO, for a channel that was last updated when the server version was ancient, like 3 or something.

But as it’s possible that InheritedMessage.of(context) will be used in other widgets, it's good to wait for #488 as you mentioned.

Yep, this is also true :) it'll be nice to be systematic about it.

Am I reading web's sort_streams correctly that it also considers channel descriptions in the filtering and ranking? I don't personally think we need to do that, but it probably deserves a mention here.

I'd prefer not to add and use tryCast for this, and instead do something like:

which is equivalent and doesn't add a step for the reader to interpret where null comes from and what it means. It also separates the main, headline logic from the rest, corresponding to the dartdoc's choice of what goes in its first line vs. the body:

  /// Comparator that puts subscribed channels before unsubscribed ones.
  ///
  /// For subscribed channels, it puts them in the following way:
  ///   pinned unmuted > unpinned unmuted > pinned muted > unpinned muted

(also nit: s/way/order/ in that dartdoc)

This is a very reasonable definition of "weekly traffic" 🙂 and so isn't likely to bitrot i.e. become incorrect over time. But since we're just using ZulipStream.streamWeeklyTraffic directly (no computations on it), let's leave that field's definition as the single place where we write its definition, so we only have to change that one thing if the meaning changes.

…I see that we haven't actually written down the field's meaning, which we might've done in dartdoc on the field. But that's quite normal and appropriate; by leaving it blank, we mean to defer to the API documentation, which is linked in the class ZulipStream dartdoc.

This should be awaited; similarly in a few other places.

Thanks to the discarded_futures lint for catching this, actually; I was playing with it for #731 this morning 🙂

nit: bump on #1902 (comment)

nit: bump on #1902 (comment)

Hmm, streams is deprecated and may be removed, so I think we should treat it as valid if streams is absent and stream_ids is present. I'd want our tests to tolerate that form without failing.

I took a look and found a bug in _readChannelIds in this revision 🙂:

diff --git lib/api/model/events.dart lib/api/model/events.dart
index 6a0d9ffa4..4d9c5121c 100644
--- lib/api/model/events.dart
+++ lib/api/model/events.dart
@@ -622,7 +622,7 @@ class ChannelDeleteEvent extends ChannelEvent {
   // TODO(server-10) simplify away; rely on stream_ids
   static List<int> _readChannelIds(Map<dynamic, dynamic> json, String key) {
     final channelIds = json['stream_ids'] as List<dynamic>?;
-    if (channelIds != null) channelIds.map((id) => id as int).toList();
+    if (channelIds != null) return channelIds.map((id) => id as int).toList();
 
     final channels = json['streams'] as List<dynamic>;
     return channels

Sure! Sounds like that would help us avoid a bug like the one fixed here.

Also could you add test cases for the strings "#124"

Ah! It seems GitHub mangled my comment: I wrote zulip/zulip-flutter#124, but it linkified it the same way as it linkifies #124, because this is a PR in zulip/zulip-flutter.

Interesting. Looks like web also does the corresponding thing for @-mentions: https://github.com/zulip/zulip/blob/1e78447c5/web/src/composebox_typeahead.ts#L516-L531

function filter_mention_name(current_token: string): string | undefined {
    if (current_token.startsWith("**")) {
        current_token = current_token.slice(2);
    } else if (current_token.startsWith("*")) {
        current_token = current_token.slice(1);
    }
    if (current_token.includes("*")) {
        return undefined;
    }

    // Don't autocomplete if there is a space following an '@'
    if (current_token.startsWith(" ")) {
        return undefined;
    }
    return current_token;
}

This is maybe more important for channel/topic autocomplete, right? Because (once the topic part is implemented) you might backspace as part of figuring out how to get just a channel link without a topic. But this is a good prompt to file an issue and add a TODO for doing this with @-mention autocomplete; would you do those?

Sure, that would be an improvement. I have always missed that feature for @-mentions; filed #1967.

I think we normally show "(unknown channel)" in italics, to distinguish it from a potential channel with that name.

The rows have 44px height in the Figma, but this gives 32px height (unless increased via the device text-size setting).

To bring it up to 44px, we could structure it similarly to MentionAutocompleteItem—

—which could be helpful in a potential future where we made a generic AutocompleteItem widget that serves both kinds of autocomplete.

Just channelLink, I think; the "syntax" part is implied. (As in existing functions in this file, like quoteAndReply which creates quote-and-reply syntax, or userMention which creates user-mention syntax.)

channel: Finish channel link autocomplete for compose box

Could you separate this special character-replacement logic into its own commit? I want to see if we can ground our reasoning in API documentation. As far as that goes, there's nothing "invalid" or "faulty" about these characters appearing in channel names.

This comment isn't right, because this is setting the store local that was declared in the outer group. That one gets overwritten in that group's other tests above.

In general it's best if the code outside of test cases (outside of test bodies) never creates large objects like instances of PerAccountStore. This code is declaring the tests, not part of running any specific test, so it runs unconditionally even when you use --name to filter down which tests run.

Hence why the "MentionAutocompleteQuery ranking" group (a) declares its own store local, (b) leaves it to be initialized lazily by a test case.

(cont'd from #1902 (comment))

This is a test for ChannelStore.compareChannelsByName. But it still doesn't test that the channel-link autocomplete code ever actually compares by name 🙂

There should be a test here that would fail if this line in the implementation:

    return ChannelStore.compareChannelsByName(a, b);

were removed (replaced with return 0;).

Conversely there's no need for this test. That's the job of tests in channel_test.dart.

Cool, this structure looks right.

This null case for traffic feels like kind of an odd edge case. I think this test would be simpler to think about if those null values were all replaced with 0 — it's clear that a channel with zero traffic should rank last on the traffic criterion.

And "weekly traffic defined" isn't really a criterion we use in the ranking — we're treating null equivalent to zero, after all. So I think this item can be dropped from the list.

nit:

compose: Introduce PerAccountStore in ComposeController

Instead can say:

compose: Move PerAccountStore up to ComposeController

That reflects the fact that although this commit adds a PerAccountStore reference on ComposeController, there already was one on the ComposeTopicController subclass.

That in turn helps reassure the reader by diminishing the scope of what's changing. When I see "introduce PerAccountStore", I have to worry: hmm, is that reference going to go out of date? Should we be doing something else to avoid keeping such a reference around? IOW, the sort of concerns that @chrisbobbe raised above at #1902 (comment) 🙂

When the verb is instead "move", though, then that's reassuring because it means that if there is such a problem it's not entirely a new problem. Also that we likely have some logic (as it turns out we do, per that thread) for dealing with the problem already, which hopefully resolves it (as indeed I think it does here).

In general, it's reassuring to have a given change be smaller rather than larger. So when you can write the commit message to clarify that's the case, with verbs like "move" or "extract" rather than "introduce" or "add", that's helpful.

nit:

nit: This comment should move to _maxLookbackForAutocompleteIntent, as it's basically explaining what that's about.

Hmm, I worry this would be slow: it's doing lookahead ((?!…)) at every point in the string.

How about this version?

This new version works for most cases, but not when there is * at the end of the query, for example, it will not pass the following test case:

doTest('~#**abc*^', channelLink('abc*'));

I agree that this regex branch would be somewhat slower, but I think it will be less noticeable as we limit the number of characters we look for a match in. Also this regex branch will be executed less often than the other one (query starting without **).
Please let me know if I am missing something and we should avoid this slow regex branch. 🙂

OK. Is that case desirable, though?

Seems like the main way it would come up is: you complete an autocomplete, then change your mind and start deleting the end of it in order to redo, and there's a moment where you've deleted one of the stars but not the other. This case checks that we end up with a query like abc* — but that's not a query the user wants in that situation, because the * is there only as a result of the #-mention syntax.

So I think in that situation, abc* is wrong; abc would be OK; and having no query is also OK, because the user can get to abc by just hitting backspace one more time.

If we did want to recognize a query abc* in that case, the regex can be adapted to do that by e.g. adding a branch \*$:

      + r'\*\*(?!\s)((?:[^*]|\*[^*]|\*$)*)'

Also this regex branch will be executed less often than the other one (query starting without **).

Hmm, on the contrary: after the user successfully uses this feature, I believe this branch will get invoked for each character they type after that. (Until they get to the max lookback distance after the #, which is probably around 100 characters after the end of the #-mention.)